Look up operator overloads by CXXOperatorName in GetFunctionsUsingName - #996
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #996 +/- ##
==========================================
+ Coverage 86.53% 86.56% +0.03%
==========================================
Files 23 23
Lines 5608 5621 +13
==========================================
+ Hits 4853 4866 +13
Misses 755 755
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| if (!DName) | ||
| DName = &Ctx.Idents.get(name); |
There was a problem hiding this comment.
Can we use LookupOperatorName instead of the string comparisons?
Something similar to:
clang/lib/Sema/SemaCoroutine.cpp-829-ExprResult Sema::BuildOperatorCoawaitLookupExpr(Scope *S, SourceLocation Loc) {
clang/lib/Sema/SemaCoroutine.cpp-830- DeclarationName OpName =
clang/lib/Sema/SemaCoroutine.cpp-831- Context.DeclarationNames.getCXXOperatorName(OO_Coawait);
clang/lib/Sema/SemaCoroutine.cpp-832- LookupResult Operators(*this, OpName, SourceLocation(),
clang/lib/Sema/SemaCoroutine.cpp:833: Sema::LookupOperatorName);
clang/lib/Sema/SemaCoroutine.cpp-834- LookupName(Operators, S);
There was a problem hiding this comment.
Answer from Claude:
Good idea, but LookupOperatorName doesn't quite fit here, for two reasons:
- It's non-member-only. That lookup kind resolves to Decl::IDNS_NonMemberOperator (SemaLookup.cpp:233) and explicitly bails out of class-member lookup (
SemaLookup.cpp:2486); Sema::LookupOverloadedOperatorName documents this as "all member functions are ignored." Member operators (CXXMethodDecl) only carry IDNS_Ordinary — setNonMemberOperator() is applied solely to operators in a non-record context. This entry point needs to find member operators too (e.g. WithOps::operator==, operator[], operator(), which the test covers), so it has to stay on LookupOrdinaryName. - It wouldn't remove the string mapping anyway. LookupOperatorName is a lookup kind, not a name parser — like the SemaCoroutine example, you still build the DeclarationName from a known OO_* via getCXXOperatorName. We only have the textual spelling ("operator=="), and clang exposes no spelling→OverloadedOperatorKind helper (the parser maps operator tokens, not strings). So OperatorKinds.def is the canonical reverse map.
I've pulled the mapping into a small getCXXOperatorDeclName helper with a comment explaining the above, which should make the intent clearer at the call site.
Confirmed empirically: swapping to LookupOperatorName keeps the namespace (free-operator) cases passing but drops all four member-operator assertions (WithOps::operator==, operator+=, operator[], operator()) to 0 matches.
There was a problem hiding this comment.
Makes sense. Consider fixing the comments and clang-tidy. In the ci summary you can find the full clang-tidy json and give it to claude for a one shot fix.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (Spelling == (OpSpelling)) \ | ||
| return Ctx.DeclarationNames.getCXXOperatorName(clang::OO_##OpName); | ||
| #include "clang/Basic/OperatorKinds.def" | ||
| return DeclarationName(); |
There was a problem hiding this comment.
We probably need an undef here.
There was a problem hiding this comment.
Perhaps to appease codecov this should be some level of llvm_unreachable.
There was a problem hiding this comment.
I managed to extend the test to cover all lines!
|
clang-tidy review says "All clean, LGTM! 👍" |
GetFunctionsUsingName built its lookup key only as an identifier, so queries like "operator==" never matched: operator overloads live in the AST under CXXOperatorName, not under an Identifier. cppyy's namespace getattr fallback (Cppyy::GetMethodsFromName -> Cpp::GetFunctionsUsingName) therefore returned no candidates and getattr(ns, 'operator==') raised AttributeError. Detect names that start with "operator" followed by a non-identifier character and match the trimmed suffix against OperatorKinds.def. When it matches, build the DeclarationName via getCXXOperatorName(OO_...); otherwise fall back to the identifier lookup. Covers symbolic and multi-token overloads (==, +, (), [], new, delete, new[], delete[], ...). Names that merely start with "operator" (e.g. "operators_count") are left on the identifier path because the next character is an identifier-continue. Fixes compiler-research/cppyy#219. 🤖 Done with the help of AI.
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
GetFunctionsUsingName built its lookup key only as an identifier, so queries like "operator==" never matched: operator overloads live in the AST under CXXOperatorName, not under an Identifier. cppyy's namespace getattr fallback (Cppyy::GetMethodsFromName -> Cpp::GetFunctionsUsingName) therefore returned no candidates and getattr(ns, 'operator==') raised AttributeError.
Detect names that start with "operator" followed by a non-identifier character and match the trimmed suffix against OperatorKinds.def. When it matches, build the DeclarationName via getCXXOperatorName(OO_...); otherwise fall back to the identifier lookup. Covers symbolic and multi-token overloads (==, +, (), [], new, delete, new[], delete[], ...). Names that merely start with "operator" (e.g. "operators_count") are left on the identifier path because the next character is an identifier-continue.
Fixes compiler-research/cppyy#219.